Skip to content

Fix GH-19044: Protected properties are not scoped according to their prototype #19046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: PHP-8.4
Choose a base branch
from

Conversation

bwoebi
Copy link
Member

@bwoebi bwoebi commented Jul 5, 2025

This should properly cover all cases, especially also around set hooks which get added later on with protected(set) visibility.

Fixes #19044.

@bwoebi bwoebi requested a review from dstogov as a code owner July 5, 2025 23:27
@bwoebi bwoebi changed the base branch from master to PHP-8.4 July 5, 2025 23:28
@bwoebi bwoebi requested review from iluuu1994 and removed request for dstogov July 5, 2025 23:28
@bwoebi bwoebi force-pushed the fix-19044 branch 5 times, most recently from e8099c0 to e1008f8 Compare July 6, 2025 00:15
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for having a look at this.

@bwoebi
Copy link
Member Author

bwoebi commented Jul 6, 2025

@iluuu1994 May you please re-review now? :-)

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, I'd prefer to be inaccurate here and allow writes to protected(set) siblings (linked through a common read-only parent). I don't think this is particularly important in practice.

@bwoebi
Copy link
Member Author

bwoebi commented Jul 6, 2025

I'd prefer to be inaccurate here and allow writes to protected(set) siblings

I think we might opt into relaxing this eventually, but I would not allow more than actually needed at least for PHP 8.4. We can always revisit it, but I don't think we should do that now.

@bwoebi
Copy link
Member Author

bwoebi commented Jul 6, 2025

@iluuu1994 Alright, let's just use prototype for everything.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the perseverance. 🙂

@@ -0,0 +1,26 @@
--TEST--
GH-19044: Protected properties must be scoped according to their prototype (hooks variation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We have gh19044.phpt and gh19044-1.phpt (along with other gh19044.phpt's), which is a bit confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you'd rather have attached a number to them all? I thought first one without number and the extras get one.

Copy link
Member

@iluuu1994 iluuu1994 Jul 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, or bump all the other ones by one so we don't have one with without prefix and one with -1. Usually we'll have:

  • gh123.phpt
  • gh123-2.phpt

Or:

  • gh123-1.phpt
  • gh123-2.phpt

I prefer the latter, but either works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants